Skip to content

Proof of concept part of cpython extension using Cython#172

Closed
sloretz wants to merge 1 commit intomasterfrom
proof_of_concept_cython
Closed

Proof of concept part of cpython extension using Cython#172
sloretz wants to merge 1 commit intomasterfrom
proof_of_concept_cython

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Jan 10, 2018

This PR is meant to generate discussion on using Cython for creating parts of the cpython extension in rclpy. Cython is a superset of python that can generate C code for cpython extensions. This PR does nothing beyond successfully build (as long as sudo apt install cython3 happens first). If the outcome of discussion is consensus in favor, I think the next step would be to replace #140 with a PR using Cython.

Advantages

Disadvantages

  • New dependency on cython (how to get it on windows? Vendor package?)
  • Linter tests don't work on cython files
    • Some things a linter would catch are caught at build time, like trying to use an uninitialized variable
  • Some burden in declaring C functions that are used (see rcl.pxd)
  • cython from CMake is a little bit of a pain (it would be much easier if rclpy used a setup.py instead)

@sloretz sloretz added the in review Waiting for review (Kanban column) label Jan 10, 2018
@sloretz sloretz self-assigned this Jan 10, 2018
@sloretz sloretz changed the title Proof of concept cpython extension using Cython Proof of concept part of cpython extension using Cython Jan 10, 2018
@dirk-thomas
Copy link
Copy Markdown
Member

I am not familiar with Cython. Is it correct that the result depends on the CPython runtime? Is that limiting us on which interpreter our code works?

I have recently read about pybind11. Maybe that would be another alternative to consider.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jan 11, 2018

@dirk-thomas Yes, it limits us to the cpython interpreter. Cython generates C code that uses the CPython extension API. It also supports pypy because pypy implements a subset of that. The current state and pybind11 have the same interpreter limitation for the same reason.

I can look into pybind11 and make a list of pros and cons

@dirk-thomas
Copy link
Copy Markdown
Member

The current state and pybind11 have the same interpreter limitation for the same reason.

Thanks for the clarification.

I can look into pybind11 and make a list of pros and cons

That would be great.

@clalancette
Copy link
Copy Markdown
Contributor

In general, I'm in favor of using something that generates code and reduces the burden of python binding maintenance for us. Off the top of my head, it is probably worth surveying Cython, pybind11, boost::python, and SWIG for this purpose, and seeing which fits in with our goals the best.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jan 12, 2018

@dirk-thomas comparison to pybind11 in #173

@clalancette I think boost::python can be skipped since pybind11 is a header only fork-ish of it. I think it is safe to assume pybind11 has all of the advantages without the overhead of installing boost.

@mikaelarguedas
Copy link
Copy Markdown
Member

Thanks @sloretz for putting this together.

Agreed regarding the statement about pybind11 over boost::python. Some sugar from boost::python is not available in pybind11 but it's very c++ specific and shouldn't impact us for C bindings.
I used pybind11 recently in other projects and it was a good experience, actually more intuitive than boost::python and pretty feature complete.
I haven't played with SWIG but it could be interesting to add it to the comparison to make an educated decision.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 7, 2018

I will close this for now since it seems pybind11 is most likely, but I will wait to delete the branch until that is confirmed.

@sloretz sloretz closed this Feb 7, 2018
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Feb 7, 2018
@sloretz sloretz deleted the proof_of_concept_cython branch April 5, 2019 22:22
YuanYuYuan pushed a commit to YuanYuYuan/rclpy that referenced this pull request Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants